Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set info logging level in find_tailcuts #1349

Merged
merged 4 commits into from
Feb 13, 2025
Merged

Conversation

moralejo
Copy link
Collaborator

@moralejo moralejo commented Feb 7, 2025

Not sure if there is a better way to set it (let me know if that is the case). Doing it in the script which calls the function did not work.

Copy link

codecov bot commented Feb 7, 2025

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 72.86%. Comparing base (76540cc) to head (d46df8b).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
lstchain/scripts/lstchain_find_tailcuts.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1349      +/-   ##
==========================================
- Coverage   72.86%   72.86%   -0.01%     
==========================================
  Files         137      137              
  Lines       14515    14516       +1     
==========================================
  Hits        10577    10577              
- Misses       3938     3939       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@morcuended
Copy link
Member

morcuended commented Feb 7, 2025

@morcuended Are the loggers of the script and of the called functions independent? Should I pass the logging level explicitly as an argument of find_tailcuts? I want it always set to "info", so the original code should do.

In principle not, I think it should be controlled by the level set in the main script calling the other module (see example at the top of https://docs.python.org/3/library/logging.html). Maybe need to set explicitly the level for file handler as well.

In lstchain_find_tailcuts.py

def main():
    args = parser.parse_args()

    output_dir = args.output_dir.absolute()
    output_dir.mkdir(exist_ok=True, parents=True)

    log.setLevel(logging.INFO)
    
    # Console handler
    console_handler = logging.StreamHandler(sys.stdout)
    console_handler.setLevel(logging.INFO)
    logging.getLogger().addHandler(console_handler)

    # File handler
    log_file = args.log_file or f'log_find_tailcuts_Run{run_id:05d}.log'
    log_file = output_dir / log_file
    file_handler = logging.FileHandler(log_file, mode='w')
    file_handler.setLevel(logging.INFO)
    logging.getLogger().addHandler(file_handler)
    ...

@moralejo
Copy link
Collaborator Author

moralejo commented Feb 7, 2025

I will try, thanks a lot for the info!

@moralejo
Copy link
Collaborator Author

moralejo commented Feb 7, 2025

Adding the console handler resulted in duplication of the (incomplete) output in stdout.
Then adding the setting for the file handler did not help either. Will keep trying.

@moralejo
Copy link
Collaborator Author

@morcuended I tried to follow the suggestions in the link you sent and did not help. The only way I seem to be able to activate INFO logging is inside cleaning.py

morcuended
morcuended previously approved these changes Feb 12, 2025
Copy link
Member

@morcuended morcuended left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not know the reason why. It is still strange to me that it is not controlled by the main script calling the function.

Therefore, use the definition inside the function for the time being.

@gabemery
Copy link
Collaborator

gabemery commented Feb 13, 2025

I think you need to specify which module you are specifying the log level for. For exemple this is how I set up the logging level for BaccMod in a notebook :

from baccmod import RadialAcceptanceMapCreator, Grid3DAcceptanceMapCreator
logging.getLogger('baccmod').setLevel(logging.WARNING)

The name used can be on a of any scope (package, module).
So here you could do :

logging.getLogger('lstchain.image.cleaning').setLevel(logging.INFO)
in lstchain_find_tailcuts.py

@moralejo
Copy link
Collaborator Author

I think you need to specify which module you are specifying the log level for. For exemple this is how I set up the logging level for BaccMod in a notebook :

from baccmod import RadialAcceptanceMapCreator, Grid3DAcceptanceMapCreator
logging.getLogger('baccmod').setLevel(logging.WARNING)

The name used can be on a of any scope (package, module). So here you could do :

logging.getLogger('lstchain.image.cleaning').setLevel(logging.INFO) in lstchain_find_tailcuts.py

Thanks, that's it indeed. I will do it like that.

@morcuended
Copy link
Member

morcuended commented Feb 13, 2025

Good! For me, it is still a mystery that you cannot control the level with the root logger, without having to specify explicitly the submodule, but for all lstchain modules at the same time. Maybe I'm misunderstanding something from the logging system.

@moralejo, are the rest of INFO messages from other parts of the code shown as well?

Copy link
Member

@morcuended morcuended left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better this way than defining it inside the function.

@gabemery
Copy link
Collaborator

Good! For me, it is still a mystery that you cannot control the level with the root logger, without having to specify explicitly the submodule, but for all lstchain modules at the same time. Maybe I'm misunderstanding something from the logging system.

@moralejo, are the rest of INFO messages from other parts of the code shown as well?

You can set the level for all lstchain in one go with
logging.getLogger('lstchain').setLevel(logging.INFO)

Current implementation will show the INFO messages of the script itself (level defined in line 51) and of methods in lstchain.image.cleaning' but not from elsewhere.

@morcuended
Copy link
Member

morcuended commented Feb 13, 2025

You can set the level for all lstchain in one go with logging.getLogger('lstchain').setLevel(logging.INFO)

Current implementation will show the INFO messages of the script itself (level defined in line 51) and of methods in lstchain.image.cleaning' but not from elsewhere.

I think I see now my wrong understanding.

log = logging.getLogger(__name__)
def main():
    args = parser.parse_args()
log.setLevel(logging.INFO)

setLevel here only acts over log, defined within the file scope, but not setting it anywhere else (as I thought it was doing, like if it was the root logger acting across all submodules).

Keep in mind that no other INFO message (default is WARNING) will appear outside image.cleaning or this script as @gabemery says, unless defining it lstchain module-wise.

@moralejo
Copy link
Collaborator Author

@moralejo, are the rest of INFO messages from other parts of the code shown as well?

I got the same output as when I defined the level inside the function

@moralejo
Copy link
Collaborator Author

You can set the level for all lstchain in one go with logging.getLogger('lstchain').setLevel(logging.INFO)

Current implementation will show the INFO messages of the script itself (level defined in line 51) and of methods in lstchain.image.cleaning' but not from elsewhere.

Yes, but this is what I want here.

@moralejo
Copy link
Collaborator Author

setLevel here only acts over log, defined within the file scope, but not setting it anywhere else (as I thought it was doing, like if it was the root logger acting across all submodules).

Keep in mind that no other INFO message (default is WARNING) will appear outside image.cleaning or this script as @gabemery says, unless defining it lstchain module-wise.

Ok, thanks! Not a problem in this case.

@moralejo moralejo merged commit a694fbf into main Feb 13, 2025
6 of 8 checks passed
@moralejo moralejo deleted the tailcut_finder_log_info branch February 13, 2025 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants